Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hyundai CAN FD: utilize macros for common address checks #1658

Merged
merged 21 commits into from
Sep 8, 2023

Conversation

sshane
Copy link
Contributor

@sshane sshane commented Sep 7, 2023

Related: #1655

Fixes the missing SCC_CONTROL check discovered in #1656, fixes a race condition with selecting wrong button check, and allows ICE to work with long by simply removing the if statement that blocks it (future PR pending a user test and route)

@sshane sshane added car safety vehicle-specific safety code cleanup labels Sep 7, 2023
@sshane sshane marked this pull request as ready for review September 7, 2023 12:27
@sshane sshane linked an issue Sep 7, 2023 that may be closed by this pull request
@sshane
Copy link
Contributor Author

sshane commented Sep 7, 2023

@adeebshihadeh this fixes the missing SCC_CONTROL check for ICE cars, and the button race condition (due to both addresses being present on some cars like GV60 EV). It also enables us to use long for ICE cars with no additional changes as a side effect (just remove the check that sets it to false and a test).

AddrCheckStruct (name)[] = { \
HYUNDAI_CANFD_COMMON_ADDR_CHECKS((pt_bus)) \
HYUNDAI_CANFD_SCC_ADDR_CHECK((scc_bus)) \
button_msg((pt_bus)) /* cppcheck-suppress misra-c2012-20.7 */ \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's the suppression for?

Copy link
Contributor Author

@sshane sshane Sep 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misra wants all args enclosed in parenthesis for safety, but the macro is expanded incorrectly if you do (button_msg)((pt_bus))

panda/board/safety/safety_hyundai_canfd.h:135:3: error: called object is not a function or function pointer
  135 |   (button_msg)((pt_bus)),  /* cppcheck-suppress misra-c2012-20.7 */             \
      |   ^

creating a macro to replace each AddrCheckStruct initialization works but adds another layer of indirection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the HYUNDAI_CANFD_CREATE_ADDR_CHECK helpers and required each safety config to define all its messages from the available address macros. Removes this misra violation, but is just a bit longer in lines. Should be clearer though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good?

board/safety/safety_hyundai_canfd.h Outdated Show resolved Hide resolved
board/safety/safety_hyundai_canfd.h Outdated Show resolved Hide resolved
Copy link
Contributor

@adeebshihadeh adeebshihadeh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

board/safety/safety_hyundai_canfd.h Outdated Show resolved Hide resolved
@sshane
Copy link
Contributor Author

sshane commented Sep 8, 2023

@sunnyhaibin adding ICE long support should be easy now, feel free to attempt it if you have spare time (let's get a route!)

@sshane sshane added the bugfix label Sep 8, 2023
@sshane
Copy link
Contributor Author

sshane commented Sep 8, 2023

test models passes on internal seglist of HKG enforcing we only update when the address was checked through the address checks! (before there was the ICE SCC_CONTROL issue and button issue)

@sshane sshane merged commit e9a0987 into master Sep 8, 2023
9 checks passed
@sshane sshane deleted the hkg-canfd-addr-check-macros branch September 8, 2023 06:52
@sunnyhaibin
Copy link
Contributor

@sunnyhaibin adding ICE long support should be easy now, feel free to attempt it if you have spare time (let's get a route!)

Will do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix car safety vehicle-specific safety code cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

safety: RX hook should not consider unknown message valid
3 participants